Skip to content

Gatsby Migration #938

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Nov 2, 2020
Merged

Gatsby Migration #938

merged 9 commits into from
Nov 2, 2020

Conversation

ardatan
Copy link
Member

@ardatan ardatan commented Oct 21, 2020

Hi everyone.
We @the-guild-org have decided to take on the work of the Gatsby migration in order to unblock all the contributions for graphql.org.
We tried to start with the amazing work that has been done in #913 by @saihaj but decided it was actually faster for us to start from scratch and finish all the things that need to be done in order to get this work merged.
We hope it will be ok with you to move the work here until we get it done.
If you disagree we are open to hear and move back to the former PR.

The main differences from the former PR are mostly around styling and making the migration pixel perfect as the current website.
Here are some differences we noticed on the former PR and made sure on this one:

  • Make live MiniGraphiQL code examples work
  • Made code highlighting work exactly as the existing website
  • Made the “How’s using GraphQL” at the bottom of the first page pixel perfect like the existing website
  • Got back the “edit this page” button
  • Got the the GraphQL Logo at the bottom of the first page
  • Sidebar on “Learn” section is now again pixel perfect as the existing website
  • Pixel perfect buttons on the “Learn” section
  • Same styling for highlighted text
  • Show title of next page in "continue reading" button

Those styling errors are obvious, just open those 3 links and move between each other:

Also, we fixed all the remaining issues from PR #913 (some just solved themselves with the new code) and backported all the fixes and PRs that were added on top of it.

We’ve also upgraded all dependencies to latest (including all the old graphql related dependencies).
(except codemirror-graphql - we’ve found an issue there and submitted a PR, @acao let me know if you have time to go through it together quickly. anyway it’s not a blocker as the former version works)

Here is a link to an up to date deployment of the branch: https://guild-gatsby-graphql-org.netlify.app

We believe this PR is in a state that is better then the current deployed version and ready to be merged.
@saihaj @leebyron @IvanGoncharov @carolstran @mikeesto please review it and if there is nothing that is worse then the current website, let’s merge and deploy the new version asap and open again the options for people to contribute!

Last thing, in order to deploy this branch we need the credentials for Netfliy (to add GitHub token for our new sorting algorithm), Algolia (to have better indexing using gatsby plugin) and Google Analytics - @leebyron @brianwarner @caniszczyk could you help us with that?

@ardatan ardatan mentioned this pull request Oct 21, 2020
@carolstran carolstran added 👾 Gatsby Related to the Gatsby migration ⚙️ Meta labels Oct 21, 2020
@carolstran
Copy link
Member

Thanks for moving the migration to another PR, I believe it'll be much more clear to review and also save the discussions we're having now 🙌🏼

Overall, I think you preserved the look and feel of the site well.

A couple of questions to start:

  1. Do you have plans to re-write the README? I noticed that you removed it without a replacement. From my understanding, a large part of the Gatsby migration is to make it easier for outside contributors to get involved, so a README is essential for that. I would say that this migration should include at least the existing README (with the Gatsby build steps) and then we can improve it in a later PR.
  2. What's the plan for the license? I noticed that you also removed this without a replacement. I know there are ongoing discussions in Bring License in line with graphql-spec #823 about the licensing, but I'm not sure if it's a good move to remove the license entirely at this point.

@ardatan
Copy link
Member Author

ardatan commented Oct 21, 2020

They were removed by mistake. I brought them back. Could you give more details like what do you expect to see in README?

@IvanGoncharov
Copy link
Member

@ardatan Can you please remove the deletion of these files from your original commit instead of adding them back as separate commit?
That way we will preserve commit history on those files.

@IvanGoncharov
Copy link
Member

@ardatan Also please review your PR and check that you migrated all files that are relevant because from a quick look I think some content is missing.

@ardatan
Copy link
Member Author

ardatan commented Oct 21, 2020

@IvanGoncharov You can choose Squash and merge while merging this PR so there will be only one single commit for this PR even if there are more commits before merging.
What kind of content do you think is missing? Could you give more details? I am not sure I follow.

@ardatan ardatan force-pushed the new-gatsby-migration branch from 751ceba to 1119742 Compare October 21, 2020 12:38
@ardatan
Copy link
Member Author

ardatan commented Oct 21, 2020

@IvanGoncharov I did another rebase to squash 2 commits into one, so there is only 1 commit for all changes like you asked for. Let me know if it's good for you.

@saihaj
Copy link
Member

saihaj commented Oct 21, 2020

What kind of content do you think is missing? Could you give more details? I am not sure I follow.

@ardatan original site has blog https://graphql.org/blog/ but this doesn’t. This is just one of the things I spotted. Will add my review soon

@carolstran
Copy link
Member

Could you give more details like what do you expect to see in README?

Minimum would be instructions for how to run locally and see/make changes (keeping in mind that many contributors will be new to Gatsby). Our README right now is... not great so we can definitely expand on it later, but the build steps are Gatsby-specific so they should be included in this PR.

@acao
Copy link
Member

acao commented Oct 21, 2020

@Ekwuno check this out!

@ardatan
Copy link
Member Author

ardatan commented Oct 22, 2020

@saihaj @carolstran Blog section is now available and I also updated the README file.

@orta
Copy link
Member

orta commented Oct 22, 2020

Wow, that blog is really well hidden - there's like no user-facing links anywhere to it :D

I've looked at entirely as a user (not read code) I gave each page on the site a look through, and they all seem to be solid 👍🏻

I only caught one bug in the search bar not taking into account that the white version of the bar (e.g. any page other than the index) will grow to be too big for the space:

Screen Shot 2020-10-22 at 3 45 54 PM
Screen Shot 2020-10-22 at 3 45 44 PM

Other than that, this PR removes the travis.yml but doesn't replace it - which I assume is going to be another PR?

@saihaj
Copy link
Member

saihaj commented Oct 22, 2020

When I click on a link on sidebar and that element is on the current page it doesn't scroll down to that element.

Kapture 2020-10-22 at 14 57 45

@saihaj
Copy link
Member

saihaj commented Oct 22, 2020

I have to refresh the users page for the CNCF script to work properly.

Kapture 2020-10-22 at 16 02 35

@saihaj
Copy link
Member

saihaj commented Oct 22, 2020

Sidebar don't match for graphql-js page

image

@ardatan
Copy link
Member Author

ardatan commented Oct 22, 2020

@saihaj Thank you :) I think they've been solved. Let me know if they persist.
@orta I think we don't need Travis or any other CI anymore because Netlify itself is able to run yarn build and take output from public then publish it. And I think that jumping issue has gone. Let me know if it still persists.

@saihaj
Copy link
Member

saihaj commented Oct 23, 2020

With what I recall discussing with @IvanGoncharov there should netlify.toml so all the config is in code and we should get deployment preview links from netlify in PRs.

@ardatan I suggest that we add some sort of linter to this project. I tired running this locally and there are many Warning: Each child in a list should have a unique "key" prop.. I think config from graphql-js eslint can be customized @IvanGoncharov what do you suggest?

Since the gastby-brower.js is empty file I suggest we delete it

@ardatan
Copy link
Member Author

ardatan commented Oct 23, 2020

@saihaj I think the issues you wrote above are gone, right? In addition, I added netlify.toml file, removed gatsby-browser.js file and fixed key prop warnings. Do you think linter configuration should be part of this PR?

@saihaj
Copy link
Member

saihaj commented Oct 23, 2020

I think it should be added in here then we can get all related fixes (if needed) in this PR.

@saihaj saihaj mentioned this pull request Nov 3, 2020
@utamori utamori mentioned this pull request Nov 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
👾 Gatsby Related to the Gatsby migration ⚙️ Meta
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants